Conversation
smoghe-bw
changed the title
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…dd integration test for endpoint management
…pointsController and update tests accordingly
… update APIController to use new auth method
…eController; update APIController to use generic auth method
…thod and using direct exception messages
…o implement configureBrtcAuth for BRTC endpoints
…equest errors in BaseController
…and remove global basic auth interference
…prove cURL initialization
…d streamline authorization header setup
…uth to prevent interference with API requests
…e APIController references
…ve array for clarity
…ody::form for improved request handling
…g format for consistency
…ride during token requests
…pe in BaseController
| */ | ||
| protected function configureAuth(&$headers, $authType) | ||
| { | ||
| if (!empty($this->config->getAccessToken()) && |
There was a problem hiding this comment.
why is all of this deleted? removing this is essentially removing OAuth support from every endpoint in the SDK, which we definitely do not want to do. All of this needs to be brought back, BRTC should be able to use this in its API calls to also support OAuth. We'll probably need to set up the username/password stuff in the configuration and then add logic to the configure auth function to set those as a fallback
| * | ||
| * @param array $headers The headers for the request (passed by reference) | ||
| */ | ||
| protected function configureOAuth2Auth(&$headers) |
There was a problem hiding this comment.
this is unnecessary, the SDK already supports OAuth, so we don't need a BRTC specific oauth function, we need to remove this and use the pre-existing logic that was in the configureAuth function
|
|
||
| require_once "Verb.php"; | ||
|
|
||
| class Connect extends Verb { |
There was a problem hiding this comment.
this is missing the eventCallbackUrl attribute
|
|
||
| class Connect extends Verb { | ||
| /** | ||
| * @var array |
There was a problem hiding this comment.
| * @var array | |
| * @var array(Endpoint) |
| */ | ||
| public function toBxml(DOMDocument $doc): DOMElement { | ||
| $element = $doc->createElement("Connect"); | ||
| foreach ($this->endpoints as $endpoint) { |
There was a problem hiding this comment.
id like to add the isset check we do in other verbs here
| $this->assertNotNull($createResp->endpointId); | ||
| $this->assertEquals('WEBRTC', $createResp->type); |
There was a problem hiding this comment.
we need to be asserting as many fields on the response as possible, feel free to copy what i did for the tn lookup tests
| $this->assertEquals('WEBRTC', $createResp->type); | ||
|
|
||
| // List endpoints | ||
| try { |
There was a problem hiding this comment.
same as above for the try catch and including more assertions
| $this->assertContains($createResp->endpointId, $ids, 'Created endpoint should be in list'); | ||
|
|
||
| // Get endpoint | ||
| try { |
| // $voiceClient->updateEndpointBxml($accountId, $createResp->endpointId, $bxml); | ||
|
|
||
| // Delete endpoint | ||
| try { |
| $this->assertEquals(204, $deleteResp->getStatusCode()); | ||
| } | ||
|
|
||
| public function testCreateEndpointResponseFields() { |
There was a problem hiding this comment.
this and every test after are all unnecessary, we need to be asserting as many fields as possible in our 1 test run, like I mentioned in one of the other SDKs, these tests are strictly to test the SDK functionality, not the API.
3 participants
No description provided.